Skip to content
This repository has been archived by the owner on Sep 6, 2022. It is now read-only.

Dot Keys environment missing param #795

Merged
merged 2 commits into from
May 4, 2018
Merged

Dot Keys environment missing param #795

merged 2 commits into from
May 4, 2018

Conversation

nakhbari
Copy link
Contributor

@nakhbari nakhbari commented May 2, 2018

Fixes the following error i was getting

bundler: failed to load command: rackup (/Users/nakhbari/.rbenv/versions/2.3.3/bin/rackup)
ArgumentError: wrong number of arguments (given 1, expected 2)
  /Users/nakhbari/.rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/gems/dotenv-2.4.0/lib/dotenv/environment.rb:7:in `initialize'
  /Users/nakhbari/Documents/fastlane/ci/app/services/dot_keys_variable_service.rb:34:in `new'
  /Users/nakhbari/Documents/fastlane/ci/app/services/dot_keys_variable_service.rb:34:in `reload_dot_env!'

@KrauseFx
Copy link
Member

KrauseFx commented May 2, 2018

Yes, this is needed for the new bundler, I merged this back with #740 and was reverted back with #757

Looks like we're gonna have to do runtime checking to be compatible with both version of dot env or bundler 👍

@nakhbari
Copy link
Contributor Author

nakhbari commented May 2, 2018

@KrauseFx Not sure i understand. Is this sufficient to merge or is there another case we're missing?

@taquitos
Copy link
Collaborator

taquitos commented May 2, 2018

Not ok to merge. We'll have to check versions of bundler and initialize them with or without the param depending on which version is installed.

@nakhbari
Copy link
Contributor Author

nakhbari commented May 2, 2018

Is there a specific version of dotkeys i can sync to that does not have the extra param? It's blocking me from spinning up the server. Bypassing it now with this change.

@KrauseFx
Copy link
Member

KrauseFx commented May 3, 2018

The change was introduced with version 2.3.0 or 2.4.0 (hard to tell from the GitHub UI) see bkeepers/dotenv@7f82f73

@nakhbari I'll update your PR to make this work for everyone 👍

@KrauseFx
Update dotenv dependency
97bc18c 
Breaking change was introduced in minor version with bkeepers/dotenv#323
@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@KrauseFx
Copy link
Member

KrauseFx commented May 3, 2018

Alright, PR is updated, and also added comments to the dot env repo bkeepers/dotenv#345

KrauseFx
KrauseFx approved these changes May 3, 2018
View reviewed changes
Copy link
Member

@KrauseFx KrauseFx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@taquitos
Copy link
Collaborator

taquitos commented May 3, 2018

@KrauseFx are you sure true is the proper value there? Your PR that originally utilized that parameter had it set to false -> https:/fastlane/ci/pull/740/files#diff-9ddf922c96bbb9ee3aeaeb8f2db5e1c6R33 .

@KrauseFx
Copy link
Member

KrauseFx commented May 3, 2018

I'm not sure, it seems like either value is fine

@KrauseFx KrauseFx merged commit bcd8f37 into master May 4, 2018
@KrauseFx
Copy link
Member

KrauseFx commented May 4, 2018

Merging this now

@KrauseFx KrauseFx deleted the nakhbari-patch-1 branch May 4, 2018 08:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@KrauseFx KrauseFx KrauseFx approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nakhbari @KrauseFx @taquitos @googlebot